-
Notifications
You must be signed in to change notification settings - Fork 167
Conversation
Thanks @deathtenk this looks plausible but seems awfully manual, I'm wondering if there is a library function to do this for us? @juzna please could you comment on this? |
yea I only did it this way because it doesn't add a dependency and as a clojure developer I try to make it a habit of trying not to muck up dependency trees. My research on pythons core time and datetime stuff revealed its fairly limited in terms of what it can do at a high level. However I'm sure someone's written a library that would account for more edge cases (perhaps pytz?). |
Yes it looks like pytz does this kind of thing but as you say it feels odd to bring in another dependency for such a small thing. I also find it strange that InstalledAppFlow is essentially giving us a UTC when the token requires local. It seems like the auth library is setting us up to fail. Or I still have not fully understood what is happening here. @juzna you say that the auth library is doing the right thing here so do you think that @deathtenk 's fix is the way to go? |
@deathtenk I'm going to run some tests with your changes. As I'm in British Summer Time right now your fix is relevant to me (in the winter GMT=UTC so the bug would not show up for me). |
Hi, if you look at python documentation of the
I didn't try to validate it with this library, but I think it should work. |
Co-authored-by: Jan Dolecek <[email protected]>
@gilesknap tested locally and this works. It's a more managed solution then the manual offset I was doing before, go ahead and test it on your end. |
Thanks both will run some tests here in BST. |
The test failures are due to |
Testing looks good. merging... Thanks @deathtenk and @juzna |
for issue: #413 , basically all this does is get the system timezone offset in seconds from
time.timezone
and subtracts it from expiry.timestamp. Also accounts for DST.